feat(explore): metrics and filters controls redesign #12095

kgabryje · 2020-12-17T10:52:26Z

SUMMARY

Redesign Metrics and Filters controls according to design in https://www.figma.com/file/JWaGztdhZS0kS5ruG7x9tB/Control-Panel?node-id=195%3A36068

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TEST PLAN

ADDITIONAL INFORMATION

CC: @villebro @junlincc

codecov-io · 2020-12-17T12:04:28Z

Codecov Report

Merging #12095 (197f5da) into master (2302adb) will decrease coverage by 3.95%.
The diff coverage is 64.97%.

@@            Coverage Diff             @@
##           master   #12095      +/-   ##
==========================================
- Coverage   67.55%   63.60%   -3.96%     
==========================================
  Files         959      964       +5     
  Lines       47154    47407     +253     
  Branches     4609     4630      +21     
==========================================
- Hits        31857    30153    -1704     
- Misses      15185    17071    +1886     
- Partials      112      183      +71

Flag	Coverage Δ
cypress	`?`
javascript	`62.75% <64.97%> (+0.08%)`	⬆️
python	`64.11% <ø> (-0.20%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../src/explore/components/AdhocFilterEditPopover.jsx	`58.00% <0.00%> (-18.00%)`	⬇️
...d/src/explore/components/MetricDefinitionValue.jsx	`70.00% <33.33%> (-17.50%)`	⬇️
...ntend/src/explore/components/AdhocMetricOption.jsx	`58.82% <40.00%> (-34.93%)`	⬇️
...frontend/src/explore/components/OptionControls.tsx	`60.97% <60.97%> (ø)`
...c/explore/components/AdhocMetricPopoverTrigger.tsx	`62.06% <62.06%> (ø)`
...src/explore/components/controls/MetricsControl.jsx	`84.21% <64.28%> (-5.45%)`	⬇️
...explore/components/controls/AdhocFilterControl.jsx	`62.09% <65.21%> (-15.58%)`	⬇️
...c/explore/components/AdhocFilterPopoverTrigger.tsx	`69.23% <69.23%> (ø)`
...ponents/AdhocFilterEditPopoverSimpleTabContent.jsx	`80.29% <70.00%> (-7.03%)`	⬇️
superset-frontend/src/components/Icon/index.tsx	`100.00% <100.00%> (ø)`
... and 207 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2302adb...197f5da. Read the comment docs.

villebro

This is looking AWESOME! 🎉 Some first pass comments. I encountered numerous bugs when testing this that turned out to be old bugs. I'll be opening a PR to address some of those in a second.

villebro · 2020-12-17T10:55:08Z

superset-frontend/spec/javascripts/explore/components/AdhocFilterControl_spec.jsx

 import AdhocFilter, {
  EXPRESSION_TYPES,
  CLAUSES,
 } from 'src/explore/AdhocFilter';
 import AdhocFilterControl from 'src/explore/components/controls/AdhocFilterControl';
 import AdhocMetric from 'src/explore/AdhocMetric';
 import { AGGREGATES, OPERATORS } from 'src/explore/constants';
+import { supersetTheme } from '@superset-ui/core';
+import { LabelsContainer } from '../../../../src/explore/components/OptionControls';


nit:

Suggested change

import { LabelsContainer } from '../../../../src/explore/components/OptionControls';

import { LabelsContainer } from 'src/explore/components/OptionControls';

villebro · 2020-12-17T10:59:18Z

superset-frontend/images/icons/fx.svg

+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements.  See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership.  The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License.  You may obtain a copy of the License at
+
+  http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied.  See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+<svg width="16" height="11" viewBox="0 0 16 11" fill="none" xmlns="http://www.w3.org/2000/svg">
+<path fill-rule="evenodd" clip-rule="evenodd" d="M4.82355 4.0072L3.96417 8.04041C3.81118 8.76307 3.48891 9.35388 2.99738 9.81287C2.50584 10.2719 1.99966 10.5013 1.47882 10.5013C1.19236 10.5013 0.981588 10.4468 0.846497 10.3378C0.711405 10.2287 0.64386 10.0912 0.64386 9.92517C0.64386 9.7852 0.686177 9.6615 0.770813 9.55408C0.855449 9.44666 0.977518 9.39295 1.13702 9.39295C1.23794 9.39295 1.32664 9.41573 1.40314 9.4613C1.47963 9.50688 1.54718 9.56384 1.60577 9.6322C1.65135 9.68754 1.70262 9.76241 1.75958 9.85681C1.81655 9.95121 1.86619 10.0326 1.90851 10.101C2.15916 10.0814 2.37319 9.91215 2.5506 9.59314C2.72801 9.27413 2.88181 8.8184 3.01202 8.22595L3.91046 4.0072H2.95831L3.06085 3.56287H4.00323L4.07159 3.23084C4.14972 2.85323 4.27342 2.51306 4.44269 2.21033C4.61196 1.90759 4.80402 1.65043 5.01886 1.43884C5.23045 1.23051 5.47052 1.06694 5.73907 0.94812C6.00763 0.829304 6.2656 0.769897 6.513 0.769897C6.79946 0.769897 7.01023 0.824422 7.14532 0.933472C7.28042 1.04252 7.34796 1.18005 7.34796 1.34607C7.34796 1.48604 7.30809 1.60974 7.22833 1.71716C7.14858 1.82459 7.02407 1.8783 6.8548 1.8783C6.75389 1.8783 6.666 1.85632 6.59113 1.81238C6.51626 1.76843 6.44952 1.71065 6.39093 1.63904C6.32583 1.55766 6.27374 1.48116 6.23468 1.40955C6.19562 1.33793 6.14679 1.25818 6.0882 1.17029C5.86358 1.18005 5.66502 1.32816 5.49249 1.61462C5.31997 1.90108 5.16372 2.37797 5.02374 3.04529L4.91632 3.56287H6.14191L6.03937 4.0072H4.82355ZM6.67739 5.89197C6.67739 4.42712 7.05174 3.23897 7.83299 2.20544H8.42706C7.84926 2.946 7.3976 4.51664 7.3976 5.89197C7.3976 7.27543 7.84519 8.842 8.42706 9.58256H7.83299C7.05174 8.54903 6.67739 7.36088 6.67739 5.89197ZM11.0841 6.68949H11.019L9.97736 8.34558H9.1839L10.6854 6.15239L9.16762 3.95919H10.0018L11.0434 5.59086H11.1085L12.138 3.95919H12.9315L11.4422 6.1239L12.9518 8.34558H12.1217L11.0841 6.68949ZM15.442 5.89604C15.442 7.36088 15.0677 8.54903 14.2864 9.58256H13.6924C14.2702 8.842 14.7218 7.27136 14.7218 5.89604C14.7218 4.51257 14.2742 2.946 13.6924 2.20544H14.2864C15.0677 3.23897 15.442 4.42712 15.442 5.89604Z" fill="#323232"/>
+</svg>


Nit: function_x.svg or similar might be a more explicit name, as someone (=me 😄 ) might think "effects" when seeing fx.svg

villebro · 2020-12-17T12:52:11Z

superset-frontend/spec/javascripts/explore/components/MetricsControl_spec.jsx

-    it('handles saved metrics being selected', () => {
-      const { wrapper, onChange } = setup();
-      const select = wrapper.find(Select);
-      select.simulate('change', [{ metric_name: 'sum__value' }]);
+    it('handles creating a new metric', () => {
+      const { component, onChange } = setup();
+      component.instance().onNewMetric({ metric_name: 'sum__value' });


We probably need to add the capability of selecting a saved (=datasource) metrics in the "Simple" tab so that saved metrics show up in the columns dropdown, and when selected, it hides the "Aggregate" select. Or some other mechanism.

villebro · 2020-12-17T12:59:44Z

superset-frontend/src/explore/components/controls/AdhocFilterControl.jsx

+                this.valueRenderer = (adhocFilter, index) => (
                  <AdhocFilterOption
                    adhocFilter={adhocFilter}
                    onFilterEdit={this.onFilterEdit}
                    options={this.state.options}
                    datasource={this.props.datasource}
                    partitionColumn={partitionColumn}
+                    onRemoveFilter={() => this.onRemoveFilter(index)}


Should we add key={index} here?

villebro · 2020-12-17T13:02:56Z

superset-frontend/src/explore/components/controls/MetricsControl.jsx

-function getDefaultAggregateForColumn(column) {
-  const { type } = column;
-  if (typeof type !== 'string') {
-    return AGGREGATES.COUNT;
-  }
-  if (type === '' || type === 'expression') {
-    return AGGREGATES.SUM;
-  }
-  if (
-    type.match(/.*char.*/i) ||
-    type.match(/string.*/i) ||
-    type.match(/.*text.*/i)
-  ) {
-    return AGGREGATES.COUNT_DISTINCT;
-  }
-  if (
-    type.match(/.*int.*/i) ||
-    type === 'LONG' ||
-    type === 'DOUBLE' ||
-    type === 'FLOAT'
-  ) {
-    return AGGREGATES.SUM;
-  }
-  if (type.match(/.*bool.*/i)) {
-    return AGGREGATES.MAX;
-  }
-  if (type.match(/.*time.*/i)) {
-    return AGGREGATES.COUNT;
-  }
-  if (type.match(/unknown/i)) {
-    return AGGREGATES.COUNT;
-  }
-  return null;
-}


I wonder if we could still leverage this logic for setting the default aggregate if it's unset and the user chooses the column first (the usual case)?

junlincc · 2020-12-17T16:41:33Z

@kgabryje much better UI!! many thanks for making it happen!

villebro · 2020-12-17T18:02:43Z

@kgabryje during testing I noticed some strange behavior, some of which was actually present from before, like inconsistent conversion of simple filters to sql filters and labels and how the isValid() function deals with missing values, causing false positives. I have a follow up PR that's mostly done which makes more sense to open after this is merged. So I suggest merging this when you feel this is ready and has had some more review and I'll follow up with a PR to fix some of those inconsistencies.

ktmud · 2020-12-17T18:29:57Z

This looks so nice. Can't wait to get rid of react-select!

mihir174 · 2020-12-17T22:00:31Z

Hey all, here are design revisions that I just discussed with @villebro and @junlincc to allow users to add saved metrics that are defined in the dataset - https://www.figma.com/file/JWaGztdhZS0kS5ruG7x9tB/Control-Panel?node-id=195%3A36068

Since there are 3 ways to add a metric, it would be confusing if 2 or more of these ways were accessed from the same set of controls. Having one tab per way of adding seems like the most intuitive solution.
@kgabryje

junlincc · 2020-12-17T22:12:22Z

we are temporarily losing the saved metrics which used to show in the Select Column dropdown. we will bring those back by implementing above design @mihir174 proposed ^^^^^ ASAP in a separate PR in the following week.

cc: @ktmud @graceguo-supercat @etr2460

villebro

As this is a major step on the SIP-34 roadmap, we're going to merge this to unblock other PRs that are dependent on this functionality. There are some known limitations in this implementation, but these will be addressed (and improved upon) in follow up PRs that are set to arrive shortly.

ktmud · 2020-12-17T22:15:43Z

👏

* upstream/master: (55 commits) feat(explore): time picker enhancement (apache#11418) feat: update alert/report icons and column order (apache#12081) feat(explore): metrics and filters controls redesign (apache#12095) feat(alerts/reports): add refresh action (apache#12071) chore: add latest tag action (apache#11148) fix(reports): increase crontab size and alert fixes (apache#12056) Small typo fix in Athena connection docs (apache#12099) feat(queries): security perm simplification (apache#12072) feat(databases): security perm simplification (apache#12036) feat(dashboards): security permissions simplification (apache#12012) feat(logs): security permissions simplification (apache#12061) chore: Remove unused CodeModal (apache#11972) Fix typescript error (apache#12074) fix: handle context-dependent feature flags in CLI (apache#12088) fix: Fix "View in SQLLab" bug (apache#12086) feat(alert/report): add 'not null' condition option to modal (apache#12077) bumping superset ui to 15.18 and deckgl to 0.3.2 (apache#12078) fix: Python dependencies in apache#11499 (apache#12079) reset active tab on open (apache#12048) fix: improve import flow UI/UX (apache#12070) ...

ktmud · 2020-12-18T08:09:54Z

Hi, finally got a chance to test this locally. Some feedbacks on the UX:

The focus state outline highlight got cut off:
The metric pills for pre-defined metrics should show metric definition, either in a info icon tooltip or when clicked:
@mihir174 what do you think of removing the "+" icon but always show the "Add metric" pill that is currently only used in the empty state. It's a little weird that two CTA living so close are basically doing the same thing.
Custom SQL tab shows an empty pair of braces when entered without any column selection:
Can we please add drag & drop to sort the order of selected metrics soon?

Not sure if these are "known limitations", but I hope it got addressed soon. since this is not behind a feature flag, we basically can't deploy if the feature is incomplete (Good thing is we don't have to ship for at least another two weeks!)

kgabryje · 2020-12-18T11:38:54Z

Thank you for your comments @ktmud, I'll work on those issues.

junlincc · 2020-12-18T11:40:52Z

1,2,4 yes. 3 not yet until @mihir174 gets back. 🙏
5 out of scope for this phase, but yes

@ktmud thanks for the comment.

zhaoyongjie · 2020-12-18T12:32:57Z

(5) can be done together with drag and drop support in Explore.
@ktmud

ktmud · 2020-12-18T16:51:33Z

Added some clarification for (5). This is basically about having full feature parity of #9627 which implements #7129.

junlincc · 2020-12-18T16:59:06Z

got it, i misunderstood. will get to it.

* Redesign metrics control * Redesign filters control * Bugfixes * Fix unit tests * Fix tests * Code review fixes

ktmud · 2021-01-16T02:29:04Z

@mihir174 what do you think of removing the "+" icon but always show the "Add metric" pill that is currently only used in the empty state. It's a little weird that two CTA living so close are basically doing the same thing.

Bump this one up. We've had user report that they didn't know how to add a second metric. So this seems to be another side-effect of having two CTAs.

cc @junlincc @etr2460

kgabryje added 4 commits December 16, 2020 18:41

Redesign metrics control

9dec3b4

Redesign filters control

2ef052d

Bugfixes

a5d73dc

Fix unit tests

b20d83b

superset-github-bot bot added the polidea label Dec 17, 2020

pull-request-size bot added the size/XXL label Dec 17, 2020

villebro reviewed Dec 17, 2020

View reviewed changes

kgabryje added 2 commits December 17, 2020 14:28

Fix tests

3a174c4

Code review fixes

197f5da

junlincc self-requested a review December 17, 2020 16:35

junlincc approved these changes Dec 17, 2020

View reviewed changes

villebro changed the title ~~feat: Metrics and Filters controls redesign~~ feat(explore): metrics and filters controls redesign Dec 17, 2020

villebro approved these changes Dec 17, 2020

View reviewed changes

villebro merged commit b61e243 into apache:master Dec 17, 2020

kgabryje mentioned this pull request Dec 18, 2020

feat: Add Saved Metrics tab to metrics popover #12123

Merged

6 tasks

villebro pushed a commit to preset-io/superset that referenced this pull request Jan 7, 2021

feat(explore): metrics and filters controls redesign (apache#12095)

234639b

* Redesign metrics control * Redesign filters control * Bugfixes * Fix unit tests * Fix tests * Code review fixes

villebro pushed a commit to preset-io/superset that referenced this pull request Jan 7, 2021

feat(explore): metrics and filters controls redesign (apache#12095)

88ae145

* Redesign metrics control * Redesign filters control * Bugfixes * Fix unit tests * Fix tests * Code review fixes

ktmud mentioned this pull request Feb 20, 2021

[Explore][Control] Allow selecting ad-hoc metrics in filters #13249

Closed

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(explore): metrics and filters controls redesign #12095

feat(explore): metrics and filters controls redesign #12095

kgabryje commented Dec 17, 2020 •

edited

Loading

codecov-io commented Dec 17, 2020 •

edited

Loading

villebro left a comment

villebro Dec 17, 2020 •

edited

Loading

villebro Dec 17, 2020

villebro Dec 17, 2020 •

edited

Loading

villebro Dec 17, 2020

villebro Dec 17, 2020

junlincc commented Dec 17, 2020

villebro commented Dec 17, 2020

ktmud commented Dec 17, 2020

mihir174 commented Dec 17, 2020 •

edited

Loading

junlincc commented Dec 17, 2020

villebro left a comment

ktmud commented Dec 17, 2020

ktmud commented Dec 18, 2020 •

edited

Loading

kgabryje commented Dec 18, 2020 •

edited

Loading

junlincc commented Dec 18, 2020 •

edited

Loading

zhaoyongjie commented Dec 18, 2020

ktmud commented Dec 18, 2020

junlincc commented Dec 18, 2020

ktmud commented Jan 16, 2021 •

edited

Loading

	import { LabelsContainer } from '../../../../src/explore/components/OptionControls';
	import { LabelsContainer } from 'src/explore/components/OptionControls';

feat(explore): metrics and filters controls redesign #12095

feat(explore): metrics and filters controls redesign #12095

Conversation

kgabryje commented Dec 17, 2020 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov-io commented Dec 17, 2020 • edited Loading

Codecov Report

villebro left a comment

Choose a reason for hiding this comment

villebro Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

villebro Dec 17, 2020

Choose a reason for hiding this comment

villebro Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

villebro Dec 17, 2020

Choose a reason for hiding this comment

villebro Dec 17, 2020

Choose a reason for hiding this comment

junlincc commented Dec 17, 2020

villebro commented Dec 17, 2020

ktmud commented Dec 17, 2020

mihir174 commented Dec 17, 2020 • edited Loading

junlincc commented Dec 17, 2020

villebro left a comment

Choose a reason for hiding this comment

ktmud commented Dec 17, 2020

ktmud commented Dec 18, 2020 • edited Loading

kgabryje commented Dec 18, 2020 • edited Loading

junlincc commented Dec 18, 2020 • edited Loading

zhaoyongjie commented Dec 18, 2020

ktmud commented Dec 18, 2020

junlincc commented Dec 18, 2020

ktmud commented Jan 16, 2021 • edited Loading

kgabryje commented Dec 17, 2020 •

edited

Loading

codecov-io commented Dec 17, 2020 •

edited

Loading

villebro Dec 17, 2020 •

edited

Loading

villebro Dec 17, 2020 •

edited

Loading

mihir174 commented Dec 17, 2020 •

edited

Loading

ktmud commented Dec 18, 2020 •

edited

Loading

kgabryje commented Dec 18, 2020 •

edited

Loading

junlincc commented Dec 18, 2020 •

edited

Loading

ktmud commented Jan 16, 2021 •

edited

Loading